-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[clang-tidy] Fix modernize-use-constraints
crash on uses of nonstandard enable_if
s
#152938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…dard `enable_if`s
@llvm/pr-subscribers-clang-tools-extra Author: Victor Chernyakin (localspook) ChangesFixes #152868. See that issue for details. Full diff: https://github.com/llvm/llvm-project/pull/152938.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
index e9b96c4016af6..ab25fb675552a 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
@@ -8,6 +8,7 @@
#include "UseConstraintsCheck.h"
#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclTemplate.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Lex/Lexer.h"
@@ -78,6 +79,15 @@ matchEnableIfSpecializationImplTypename(TypeLoc TheType) {
if (!TD || TD->getName() != "enable_if")
return std::nullopt;
+ const TemplateParameterList *Params = TD->getTemplateParameters();
+ if (Params->size() != 2)
+ return std::nullopt;
+
+ const auto *FirstParam =
+ dyn_cast<NonTypeTemplateParmDecl>(Params->getParam(0));
+ if (!FirstParam || !FirstParam->getType()->isBooleanType())
+ return std::nullopt;
+
int NumArgs = SpecializationLoc.getNumArgs();
if (NumArgs != 1 && NumArgs != 2)
return std::nullopt;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 2de2818172850..f6aedb299e456 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -136,6 +136,11 @@ Changes in existing checks
- Improved :doc:`misc-header-include-cycle
<clang-tidy/checks/misc/header-include-cycle>` check performance.
+- Fixed a :doc:`modernize-use-constraints
+ <clang-tidy/checks/modernize/use-constraints>` crash on uses of
+ nonstandard ``enable_if``s with a signature different from
+ ``std::enable_if`` (such as ``boost::enable_if``).
+
- Improved :doc:`modernize-use-designated-initializers
<clang-tidy/checks/modernize/use-designated-initializers>` check to
suggest using designated initializers for aliased aggregate types.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-constraints.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-constraints.cpp
index 3bcd5cd74024e..d61073c1aac3f 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-constraints.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-constraints.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy -std=c++20 %s modernize-use-constraints %t -- -- -fno-delayed-template-parsing
+// RUN: %check_clang_tidy -std=c++20-or-later %s modernize-use-constraints %t -- -- -fno-delayed-template-parsing
// NOLINTBEGIN
namespace std {
@@ -756,3 +756,32 @@ abs(const number<T, ExpressionTemplates> &v) {
}
}
+
+template <typename T>
+struct some_type_trait {
+ static constexpr bool value = true;
+};
+
+// Fix-its are offered even for a nonstandard enable_if.
+namespace nonstd {
+
+template <bool Condition, typename T = void>
+struct enable_if : std::enable_if<Condition, T> {};
+
+}
+
+template <typename T>
+typename nonstd::enable_if<some_type_trait<T>::value, void>::type nonstd_enable_if() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use C++20 requires constraints instead of enable_if [modernize-use-constraints]
+// CHECK-FIXES: {{^}}void nonstd_enable_if() requires some_type_trait<T>::value {}{{$}}
+
+// But only if the nonstandard enable_if has the same signature as the standard one.
+namespace boost {
+
+template <typename Condition, typename T = void>
+struct enable_if : std::enable_if<Condition::value, T> {};
+
+}
+
+template <typename T>
+typename boost::enable_if<some_type_trait<T>, void>::type boost_enable_if() {}
|
@llvm/pr-subscribers-clang-tidy Author: Victor Chernyakin (localspook) ChangesFixes #152868. See that issue for details. Full diff: https://github.com/llvm/llvm-project/pull/152938.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
index e9b96c4016af6..ab25fb675552a 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
@@ -8,6 +8,7 @@
#include "UseConstraintsCheck.h"
#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclTemplate.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Lex/Lexer.h"
@@ -78,6 +79,15 @@ matchEnableIfSpecializationImplTypename(TypeLoc TheType) {
if (!TD || TD->getName() != "enable_if")
return std::nullopt;
+ const TemplateParameterList *Params = TD->getTemplateParameters();
+ if (Params->size() != 2)
+ return std::nullopt;
+
+ const auto *FirstParam =
+ dyn_cast<NonTypeTemplateParmDecl>(Params->getParam(0));
+ if (!FirstParam || !FirstParam->getType()->isBooleanType())
+ return std::nullopt;
+
int NumArgs = SpecializationLoc.getNumArgs();
if (NumArgs != 1 && NumArgs != 2)
return std::nullopt;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 2de2818172850..f6aedb299e456 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -136,6 +136,11 @@ Changes in existing checks
- Improved :doc:`misc-header-include-cycle
<clang-tidy/checks/misc/header-include-cycle>` check performance.
+- Fixed a :doc:`modernize-use-constraints
+ <clang-tidy/checks/modernize/use-constraints>` crash on uses of
+ nonstandard ``enable_if``s with a signature different from
+ ``std::enable_if`` (such as ``boost::enable_if``).
+
- Improved :doc:`modernize-use-designated-initializers
<clang-tidy/checks/modernize/use-designated-initializers>` check to
suggest using designated initializers for aliased aggregate types.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-constraints.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-constraints.cpp
index 3bcd5cd74024e..d61073c1aac3f 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-constraints.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-constraints.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy -std=c++20 %s modernize-use-constraints %t -- -- -fno-delayed-template-parsing
+// RUN: %check_clang_tidy -std=c++20-or-later %s modernize-use-constraints %t -- -- -fno-delayed-template-parsing
// NOLINTBEGIN
namespace std {
@@ -756,3 +756,32 @@ abs(const number<T, ExpressionTemplates> &v) {
}
}
+
+template <typename T>
+struct some_type_trait {
+ static constexpr bool value = true;
+};
+
+// Fix-its are offered even for a nonstandard enable_if.
+namespace nonstd {
+
+template <bool Condition, typename T = void>
+struct enable_if : std::enable_if<Condition, T> {};
+
+}
+
+template <typename T>
+typename nonstd::enable_if<some_type_trait<T>::value, void>::type nonstd_enable_if() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use C++20 requires constraints instead of enable_if [modernize-use-constraints]
+// CHECK-FIXES: {{^}}void nonstd_enable_if() requires some_type_trait<T>::value {}{{$}}
+
+// But only if the nonstandard enable_if has the same signature as the standard one.
+namespace boost {
+
+template <typename Condition, typename T = void>
+struct enable_if : std::enable_if<Condition::value, T> {};
+
+}
+
+template <typename T>
+typename boost::enable_if<some_type_trait<T>, void>::type boost_enable_if() {}
|
Latest commit fixes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, as a final sanity check, could we add tests with 1 param in enable_if and specialized function like template<> enable_if<true, int>::value foo()
The check right now skips non-dependent |
If there isn't such a test right now, I'd say yes. |
Looks like we have tests for non-dependent llvm-project/clang-tools-extra/test/clang-tidy/checkers/modernize/use-constraints.cpp Lines 71 to 75 in 76f1c7a
|
Fixes #152868. See that issue for details.
Fixes #123726.